Skip to content

Enhance functionality of the LIMA_SHELLENV_* filtering mechanism for limactl shell ... --preserve-env #4101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 19, 2025
Merged

Enhance functionality of the LIMA_SHELLENV_* filtering mechanism for limactl shell ... --preserve-env #4101

merged 1 commit into from
Oct 19, 2025

Conversation

olamilekan000
Copy link
Contributor

@olamilekan000 olamilekan000 commented Sep 26, 2025 •
edited
Loading 

change improves the flexibility of the limactl shell command when used with the 
--preserve-env flag by validating environment variable patterns to only 
allow alphanumeric characters and underscores also allowing 
more flexible environment variable filtering.

Fixes
#4036

AkihiroSuda
AkihiroSuda previously requested changes Sep 26, 2025
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description is not clear to explain what is improved

@olamilekan000 olamilekan000 force-pushed the improve-flexibitlty-of-shell branch 4 times, most recently from f570e72 to 3905a98 Compare September 26, 2025 21:57
jandubois
jandubois reviewed Sep 26, 2025
View reviewed changes
jandubois
jandubois reviewed Sep 26, 2025
View reviewed changes
@olamilekan000 olamilekan000 force-pushed the improve-flexibitlty-of-shell branch from 3905a98 to 2288670 Compare September 27, 2025 09:14
jandubois
jandubois requested changes Sep 30, 2025
View reviewed changes
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't actually run the code, but from reading the changes, it doesn't look like it implements the modified allow list semantics.

It is also missing tests for the changes.

@olamilekan000 olamilekan000 force-pushed the improve-flexibitlty-of-shell branch 2 times, most recently from bef31a5 to deeae2d Compare September 30, 2025 20:56
@jandubois
Copy link
Member

Please run your tests before pushing to the PR:

--- FAIL: TestFilterEnvironment (0.00s)
    --- FAIL: TestFilterEnvironment/allowlist (0.00s)
        envutil_test.go:173: assertion failed: 9 (int) != 2 (int)
FAIL
FAIL	github.com/lima-vm/lima/v2/pkg/envutil	0.188s

@olamilekan000 olamilekan000 force-pushed the improve-flexibitlty-of-shell branch 3 times, most recently from 8ec3ecc to 8428142 Compare September 30, 2025 21:40
jandubois
jandubois requested changes Oct 1, 2025
View reviewed changes
@olamilekan000 olamilekan000 force-pushed the improve-flexibitlty-of-shell branch 5 times, most recently from 28c7581 to 1907080 Compare October 1, 2025 19:43
jandubois
jandubois requested changes Oct 3, 2025
View reviewed changes
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tests need another round of simplification.

@olamilekan000 olamilekan000 force-pushed the improve-flexibitlty-of-shell branch from 1907080 to c7f611f Compare October 5, 2025 06:44
@olamilekan000 olamilekan000 force-pushed the improve-flexibitlty-of-shell branch from c7f611f to 2120deb Compare October 5, 2025 06:46
@AkihiroSuda AkihiroSuda added this to the v2.0.0 milestone Oct 10, 2025
@AkihiroSuda AkihiroSuda requested a review from a team October 17, 2025 10:02
jandubois
jandubois approved these changes Oct 19, 2025
View reviewed changes
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tests could still be tightened up, but the actual functionality seems to be working as expected.

So I'm going to merge; tests can always be refactored some more later.

hack/bats/tests/preserve-env.bats
Comment on lines +145 to +146
export NORMAL_VAR=normal_var
export UNRELATED=unrelated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think having a second unrelated variable proves anything, but it doesn't hurt.

hack/bats/tests/preserve-env.bats
Comment on lines +162 to +163
export LIMA_SHELLENV_ALLOW="SSH_*,CUSTOM*"
export LIMA_SHELLENV_BLOCK="+*TOKEN"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test case is mostly redundant with the allow list can use a , separated list with whitespace ignored one, except it also tests the "append" functionality. So these tests could be combined.

@jandubois jandubois changed the title improve flexibility of limactl shell cmd Enhance functionality of the LIMA_SHELLENV_* filtering mechanism for limactl shell ... --preserve-env Oct 19, 2025
@jandubois jandubois dismissed AkihiroSuda’s stale review October 19, 2025 22:10

The only requested change was to the PR title, which I have updated.

@jandubois jandubois merged commit 883c441 into lima-vm:master Oct 19, 2025
62 of 63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jandubois jandubois jandubois approved these changes

@AkihiroSuda AkihiroSuda Awaiting requested review from AkihiroSuda

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@olamilekan000 @jandubois @AkihiroSuda